Add typings that work with current TS version #63
Conversation
|
❌ Build es6-promise-pool 203 failed (commit 17106e522d by @haslers) |
|
Fails with error TS2304: Cannot find name 'EventTarget' |
|
@haslers , this version is working for me and does not depend on external typings (it only exposes /* eslint no-use-before-define:0, semi:0, space-infix-ops:0 */
declare namespace PromisePool {
type EventName = 'fulfilled' | 'rejected';
interface Options<T> {
promise?: Promise<T>;
}
interface Event<T> {
target: PromisePool<T>;
type: EventName;
data: T;
}
type Producer<T> = IterableIterator<Promise<T>> | Promise<T> | (() => PromiseLike<T> | null | void) | T;
type Listener<T> = (param: Event<T>) => any;
}
declare class PromisePool<T> {
constructor(
source: () => PromisePool.Producer<T>,
concurrency: number,
options?: PromisePool.Options<T>
);
concurrency(concurrency: number): number;
size(): number;
active(): boolean;
promise(): Promise<T> | null;
start(): Promise<T>;
addEventListener(type: PromisePool.EventName, listener: PromisePool.Listener<T>): void;
removeEventListener(type: PromisePool.EventName, listener: PromisePool.Listener<T>): void;
}
export = PromisePool; |
|
What is your setup, @aMarCruz? If |
|
@Aankhen , my tsconfig.json: {
"compilerOptions": {
"lib": ["es6"],
"module": "commonjs",
"allowJs": true,
"noImplicitReturns": true,
"outDir": "lib",
"sourceMap": true,
"target": "es6"
},
"compileOnSave": true,
"files": [
"index.d.ts"
],
"include": [
"src"
]
}from my package.json : "dependencies": {
"es6-promise-pool": "^2.5.0",
"firebase-admin": "~5.12.1",
"firebase-functions": "^1.1.0"
}This is (almost) the default setup for Google Firebase cloud functions in a node.js environment, the |
|
...my devDependencies: "devDependencies": {
"eslint": "^5.1.0",
"firebase-functions-test": "^0.1.2",
"mocha": "^5.2.0",
"ts-node": "^7.0.0",
"typescript": "^2.9.2",
"typescript-eslint-parser": "^16.0.1"
},and you're right, it is strange but TS can't find the EventTarget definition. |
|
Yeah, I copied your configuration and I get the same error unless I remove the EDIT: EDIT 2: |
|
Sorry about the radio silence here. I want to have another attempt at improving the typings. That said, I'm still not using TypeScript personally, and we've kind of drifted away from it at work as well. I'm happy to accept working typings from the community. There are currently three PRs open around typings: #40, #47, and #63 (this one). Intuitively, I'd close the older two and merge the most recent one. However, that probably shouldn't be the only criterion. Since I don't have a lot of skin in this game, I'm looking at you guys to review the PRs. Please let me know what you think. Thanks! |
|
Apart from one observation about the options, I think this PR does a good job. I’ve been using it for a while. (I don’t have any exotic requirements, though!) That said, if you’re not planning to use TypeScript here, then it would probably be better to remove the types from this repository and add them to DefinitelyTyped instead, which anyone can do. |
| declare namespace PromisePool { | ||
| export interface Options<A> { | ||
| promise?: Promise<A> |
There was a problem hiding this comment.
This indicates that promise is an instance of Promise, whereas what it should be is a function that can produce a PromiseLike. I adapted a definition from Bluebird:
declare namespace PromisePool {
interface PromiseFunction<A> {
new(callback: (resolve: (value?: A) => void, reject: (reason?: any) => void) => void): PromiseLike<A>;
}
export interface Options<A> {
promise?: PromiseFunction<A>
}
}Which made this work:
import PromisePool = require("es6-promise-pool");
import Bluebird = require("bluebird");
const options = { promise: Bluebird };
const pool = new PromisePool("test", 1, options);
const options2 = { promise: Promise };
const pool2 = new PromisePool("test", 1, options2);
|
Thanks for your feedback! I think it would make a lot of sense to migrate the typings to DefinitelyTyped. I've just commited a change that removes them from the package, but I should probably hold off on releasing a new version until you or someone else has added them to DefinitelyTyped? What's the procedure? |
|
Not at all. Thank you for a very useful library! I can add them to DefinitelyTyped (it only needs writing a few tests to go with the definitions and opening a PR), but perhaps @aappddeevv would prefer to submit the definitions themselves (with the options fixed) so that their name is in the list of authors and they’re contacted in case of issues. |
|
Sounds good! After the typings have been added to DefinitelyTyped, tsc will automatically pick them up, right? Or do I need to put anything in |
|
You don’t need to do anything; you only need a |
|
Makes sense. Could you ping me here once the typings are in DefinitelyTyped? Thanks! |
|
I’ve been trying to put an improved version of the types together for DT since the original authors don’t seem to be around, but I ran into a roadblock: I haven’t been able to figure out how to avoid flattening the output type, i.e. tell TypeScript that if Bluebird is passed in as the promise library then the output is a Bluebird promise too. No luck on Stack Overflow so far. I’m hoping someone can eventually answer. |
|
This is getting complicated. I realized my mistake, which solved the direct issue, but when you add another layer, there is a hole in the type inference that seems to make it impossible to express the types in TypeScript. |
This branch basically just copies the typings from #47 and adds them. Works better than the current typings.